Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge pir stabilization feature branch into main #2789

Merged
merged 11 commits into from
May 22, 2024

Conversation

THISISDINOSAUR
Copy link
Contributor

@THISISDINOSAUR THISISDINOSAUR commented May 16, 2024

Task/Issue URL: https://app.asana.com/0/1199230911884351/1207338264132623/f
Tech Design URL: https://app.asana.com/0/481882893211075/1207174884557414/f
CC:

Description:
This PR has the effect of mergeing four different PRs into main:
#2777
#2758
#2757
#2743
This covers significant changes to the XPC interface and how the main app uses it, the background manager, the scheduler, and the processor. See individual PRs for details.

There's no code changes here that haven't been reviewed separately as part of those PRs, so it's up to you how you want to review it code wise. The more important step at this stage is manual testing.

Steps to test this PR:

  1. Because this is a fairly significant set of changes, we need to be as thorough as we can in testing DBP generally works and really try to break it. Because of this, I'm keen that anyone testing things of their own ways to break it before trying the steps below.
  2. Edit/add profile data, and before scans finish, edit it again. Scans should start afresh, and you should see an interrupted pixel fire. Check that you don't get a scans completed notification
  3. Edit/add profile data, and before scans finish, close the app, and reopen it. The same set of manual scans should still continue, and a blocked pixel should fire.
  4. With profile data already existing but initial scans having finished, close the app and reopen it. Scheduled scans should run.
  5. With profile data already existing, and with the app closed, launch the background agent, scheduled scans should run
  6. Edit/add profile data, put the laptop to sleep/lock it/restart it, check that scans continue as expected.
  7. Check that the background agent activity lines up with the UI, I.e. the progress bar and when we sent the scans completed notification is accurate

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

aataraxiaa and others added 4 commits May 8, 2024 16:59
Task/Issue URL:https://app.asana.com/0/1199230911884351/1207175016813446/f

**Description**:This PR includes all changes relating to [this tech design
sub-task](https://app.asana.com/0/0/1207199754528648/f). Specifically:
Task/Issue URL: https://app.asana.com/0/0/1207231867963535/f
Tech Design URL: https://app.asana.com/0/0/1207198317317333/f
CC:

**Description**:
1. Moves the existing XPC interface ("DataBrokerProtectionScheduler")
from the scheduler into the BackgroundAgentManager
2. Renames said manager to AgentManager
3. Renames the XPC interface to DataBrokerProtectionAgentInterface
4. Completely changes the composition of the interface, separating it
out into two different protocols: DataBrokerProtectionAgentAppEvents,
DataBrokerProtectionAgentDebugCommands
5. As those protocols imply, changes the XPC interface methods to be
events based, to move decision making from the main app to the agent.
Except for the debug commands, which are now clearly separated as for
debugging purposes only.
6. Removes two of the many XPC layers since they were operating solely
as a passthrough (i.e. boilerplate for nothing)
7. Uses protocol composition to further cut down on the repetition in
those layers
8. Changes LoginItemScheduler to rather than be a class, a protocol that
inherits the AgentInterface
9. Removes existing completions from the XPC interface since they were
unused
10. Replaces them with new completions that are solely for reporting the
reliability of XPC

It still needs the pixels for #9, including clean up of the old pixels

**Steps to test this PR**:
1. Test DBP still works, particularly removing the login item, entirely
and starting fresh (since when testing at one point I managed to break
that)
2. Otherwise don't test too much since this PR is not intended to stand
alone/be merged into main as is.

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)
Task/Issue URL: https://app.asana.com/0/0/1207231867963533/f
Tech Design URL: https://app.asana.com/0/0/1207199754528649/f

**Description**: This PR introduces `DataBrokerProtectionQueueManager` to manage state
and logic relating to the `OperationQueue` which runs DBP operations.
Task/Issue URL:
https://app.asana.com/0/1199230911884351/1207175016813449/f
Tech Design URL:
CC:

**Description**:
1. Move all userNotification related calls and anything else unrelated
to scheduling up a layer to the agent manager
2. Removes the existing scheduler, and moves the ActivityScheduler
management to a new class
3. Make a variety of changes to scheduler behavior, such as running all
the time (as long as theirs profile data), changing scheduler frequency
to 20 minutes, removes status publishing
4. Fixes us not deleting the login item when the user deleted their data
5. As part of that, removes the dataDeleted IPC method, since it's no
longer needed
5. Reworks the agent manager and associated dependancies so it can be
unit tested
6. Adds those unit tests.

Pixels are still outstanding
But everything else should be done now (integrating with Pete's latest
PR etc)

**Steps to test this PR**:
1. Generally test that DBP works, but not too thoroughly since we will
do that for the whole feature branch at once.

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)

---------

Co-authored-by: Pete <[email protected]>
# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerJobRunner.swift
#	LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerOperation.swift
@THISISDINOSAUR
Copy link
Contributor Author

@jotaemepereira can you pay particular attention to if the sleep stuff is still as expected? The merge was tricky because of the file renames, git wanted to do some very incorrect things

Copy link
Contributor

github-actions bot commented May 17, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against f645b59

Copy link
Contributor

@aataraxiaa aataraxiaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Nothing stands out after main merge. Most scenarios work as expected. Still figuring out best way to test some pixels and scheduled scans, but others might have ideas on how to best verify these. Specifically:

  • Whats the best way to test a background scan is started when we launch the app? I tried attaching to the agent process via Xcode, then launching the app, but this results in a SIGTERM.
  • How can we test agent pixels are fired in the above scanario? If we can’t attach before launch, they won’t appear in Xcode’s console.

Either way I’ll continue to look into testing, but wanted to add my initial feedback now.

await onError(error: DataBrokerProtectionError.emailError(error as? EmailError))
}
// swiftlint:disable explicit_non_final_class
class DataBrokerOperation: Operation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing here stands out as incorrect after the main merge @THISISDINOSAUR 👍

import UserScript
import Common

protocol DataBrokerJob: CCFCommunicationDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing stands out here as incorrect, but I think (?) @jotaemepereira made changes here which were merged from main so would be good for him to have a look.

@aataraxiaa
Copy link
Contributor

Overall looks good. Nothing stands out after main merge. Most scenarios work as expected. Still figuring out best way to test some pixels and scheduled scans, but others might have ideas on how to best verify these. Specifically:

  • Whats the best way to test a background scan is started when we launch the app? I tried attaching to the agent process via Xcode, then launching the app, but this results in a SIGTERM.
  • How can we test agent pixels are fired in the above scanario? If we can’t attach before launch, they won’t appear in Xcode’s console.

Either way I’ll continue to look into testing, but wanted to add my initial feedback now.

I was able to confirm background scans ran after app launch using a combination of the Debug DB viewer and Activity Monitor. On launch opt-outs from previous scan were run. 👍🏼

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionBackgroundManager.swift
#	LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/DebugUI/DataBrokerRunCustomJSONViewModel.swift
#	LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionScheduler.swift
#	LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/Mocks.swift
@aataraxiaa
Copy link
Contributor

Tested quite a bit today with various scans and all looks good. Only thing I wasn’t sure how to verify was the interrupted pixel fire, but maybe another reviewer here could verify that. 🚀

@THISISDINOSAUR
Copy link
Contributor Author

@jotaemepereira can you pay particular attention to if the sleep stuff is still as expected? The merge was tricky because of the file renames, git wanted to do some very incorrect things

@Bunn same thing re the subscription stuff

Copy link
Contributor

@aataraxiaa aataraxiaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice find @Bunn 🚀

Copy link
Collaborator

@Bunn Bunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@jotaemepereira jotaemepereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@THISISDINOSAUR While testing manual scans, I realized the flag that we pass to the StageCalculator is now wrong, which causes some of the pixels to not be triggered when they should be.

@THISISDINOSAUR
Copy link
Contributor Author

THISISDINOSAUR commented May 20, 2024

@THISISDINOSAUR While testing manual scans, I realized the flag that we pass to the StageCalculator is now wrong, which causes some of the pixels to not be triggered when they should be.

Whereabouts? Is it the isImmediateOperation flag?

Copy link
Collaborator

@jotaemepereira jotaemepereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried again, and it is working. I do not know what happened yesterday and why the flag was false, though 🤷🏼‍♂️

@THISISDINOSAUR THISISDINOSAUR merged commit 8dd682c into main May 22, 2024
22 checks passed
@THISISDINOSAUR THISISDINOSAUR deleted the feature/pir-stabilization branch May 22, 2024 20:16
samsymons added a commit that referenced this pull request May 24, 2024
# By Elle Sullivan (8) and others
# Via Elle Sullivan (3) and GitHub (2)
* main: (26 commits)
  Autofill engagement KPIs for pixel reporting (#2806)
  autofill: don't prefix autofill email pixels with `m.mac.` (#2808)
  Bump BSK (#2807)
  Make profile selector optional (#2811)
  Add History to iOS (updated UI and rollout) (#2770)
  New autofill save & update password prompt pixels for alignment with iOS (#2801)
  Add mylife data broker (#2786)
  Increase test timeout (#2810)
  Subscription refactoring, BSK update (#2809)
  Check for entitlement in DBP agent (#2802)
  move permanent survey card to first position (#2804)
  Subscription refactoring (#2764)
  Bump version to 1.89.0 (191)
  Merge pir stabilization feature branch into main (#2789)
  Update age params for multiple brokers (#2800)
  Fix top level navigation blocks (#2792)
  Adding to the Dock automatically (#2722)
  Fix lint error
  Make activity only call completion once all work has actually finished
  Fix activity scheduler not calling completion
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants